feat: Improve Admin Settings UX and restore inline comments (#7603)#7666
Closed
AyushiGupta160604 wants to merge 2 commits into
Closed
feat: Improve Admin Settings UX and restore inline comments (#7603)#7666AyushiGupta160604 wants to merge 2 commits into
AyushiGupta160604 wants to merge 2 commits into
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Review Summary by Qodo(Agentic_describe updated until commit 868e607)Enhance Admin Settings Editor with Validation and UX Improvements
WalkthroughsDescription• Improved admin settings editor UX with monospaced font and dark theme styling • Added Tab key support for indentation and JSON syntax validation with error messages • Implemented dry-run validation button to test JSON without saving • Added prettify JSON feature with confirmation dialog (feature-flagged) • Enhanced error handling with connection checks and detailed validation feedback Diagramflowchart LR
A["Settings Page"] --> B["Enhanced Textarea"]
B --> C["Dark Theme Styling"]
B --> D["Tab Key Handler"]
A --> E["Validation Features"]
E --> F["Test JSON Button"]
E --> G["Prettify JSON Button"]
A --> H["Error Handling"]
H --> I["Connection Checks"]
H --> J["Detailed Error Messages"]
File Changes1. admin/src/App.css
|
Code Review by Qodo
Context used 1. Settings textarea not pretty-printed
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Member
|
Wow this is an awesome PR :) See Qodo feedback, I'd also add that I would recommend a "Test JSON" before we implement it, so it does a dry-run and doesn't b0rk your deployment :) |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit 868e607 |
Contributor
Author
|
I have finalized this revision by implementing the 'Test JSON' dry-run feature and addressing all bot feedback. Key updates include:
Ready for review! |
Comment on lines
+72
to
+89
| <textarea | ||
| value={settings} | ||
| className="settings" | ||
| spellCheck={false} | ||
| onKeyDown={handleKeyDown} | ||
| onChange={v => useStore.getState().setSettings(v.target.value)} | ||
| style={{ | ||
| fontFamily: '"Fira Code", "Cascadia Code", monospace', | ||
| width: '100%', | ||
| height: '500px', | ||
| padding: '15px', | ||
| backgroundColor: '#1e1e1e', | ||
| color: '#d4d4d4', | ||
| lineHeight: '1.5', | ||
| border: '1px solid #333', | ||
| resize: 'vertical' | ||
| }} | ||
| /> |
There was a problem hiding this comment.
1. Settings textarea not pretty-printed 📎 Requirement gap ≡ Correctness
The new /admin/settings UI still renders the raw settings string directly in a <textarea> without parsing and pretty-printing it for a consistently formatted display. This fails the requirement to present settings.json as a parsed/pretty-printed, well-formatted representation.
Agent Prompt
## Issue description
The `/admin/settings` page still displays the raw `settings` text directly, rather than presenting a parsed/pretty-printed representation as required.
## Issue Context
Compliance requires the admin UI to show a consistently formatted, parsed/pretty-printed view that preserves readable structure.
## Fix Focus Areas
- admin/src/pages/SettingsPage.tsx[72-89]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+72
to
+89
| <textarea | ||
| value={settings} | ||
| className="settings" | ||
| spellCheck={false} | ||
| onKeyDown={handleKeyDown} | ||
| onChange={v => useStore.getState().setSettings(v.target.value)} | ||
| style={{ | ||
| fontFamily: '"Fira Code", "Cascadia Code", monospace', | ||
| width: '100%', | ||
| height: '500px', | ||
| padding: '15px', | ||
| backgroundColor: '#1e1e1e', | ||
| color: '#d4d4d4', | ||
| lineHeight: '1.5', | ||
| border: '1px solid #333', | ||
| resize: 'vertical' | ||
| }} | ||
| /> |
There was a problem hiding this comment.
2. No inline comment mapping 📎 Requirement gap ≡ Correctness
The settings UI does not extract and associate settings.json comments with their related settings as inline annotations; it only shows a raw text blob. This does not meet the requirement to surface comments in a setting-associated way.
Agent Prompt
## Issue description
The admin settings page does not implement any extraction/mapping of comments to the corresponding settings for inline annotations/tooltips.
## Issue Context
Compliance requires surfacing documentation/comments originally present in `settings.json` in a way that is associated with the relevant setting (not just raw text).
## Fix Focus Areas
- admin/src/pages/SettingsPage.tsx[72-89]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+135
to
+144
| <IconButton | ||
| className="settingsButton" | ||
| icon={<RotateCw />} | ||
| // FIX: Stable ID for Playwright automation | ||
| data-testid="restart-etherpad-button" | ||
| title={<Trans i18nKey="admin_settings.current_restart.value" />} | ||
| onClick={() => { | ||
| settingsSocket?.emit('restartServer'); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
3. Invalid iconbutton testid prop 🐞 Bug ≡ Correctness
SettingsPage passes data-testid to IconButton, but IconButtonProps does not include it and IconButton does not forward unknown props to the underlying <button>, causing a TS build failure and preventing the attribute from being rendered. This also blocks the intended “stable selector” for E2E tests.
Agent Prompt
### Issue description
`IconButton` is a wrapper around `<button>`, but it does not accept/forward standard button attributes like `data-testid`. `SettingsPage` now passes `data-testid`, which will fail TS type-checking and will not render into the DOM.
### Issue Context
The PR added `data-testid="restart-etherpad-button"` to stabilize Playwright selectors, but the wrapper component prevents it from working.
### Fix Focus Areas
- admin/src/components/IconButton.tsx[1-17]
- admin/src/pages/SettingsPage.tsx[135-144]
### Implementation notes
- Update `IconButtonProps` to extend `React.ButtonHTMLAttributes<HTMLButtonElement>` (or at least include an index signature / explicit `'data-testid'?: string`).
- Spread remaining props onto the `<button>` so attributes like `data-testid`, `aria-*`, etc. are preserved.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
@AyushiGupta160604 see feedback from Qodo again :) |
Member
|
Test coverage is missing |
Member
|
Assuming you are using copilot yea? We're fine w/ AI just please tell it to look at https://github.com/ether/etherpad/blob/develop/AGENTS.MD |
Contributor
Author
|
Appreciate the nudge, John! I've been keeping AGENTS.MD in mind, but I’m doing a deeper dive now to make sure I haven't missed any of the finer details. I'm currently working on refactoring the IconButton to make those data-testid props actually stick—which should fix the E2E stability issues the bot flagged. I’m also drafting the missing test coverage to make sure we don't have any regressions down the line. I’ll get these pushed and move this back to 'Ready for Review' as soon as the automation looks solid! |
Member
|
This is 100% AI fwiw :) I'm not opposed to it as long as its not wasting more time than me just running the commands myself 🖌️ |
Contributor
Author
|
Yes, you are right. I have been using AI but have been working on code and reviewing the things myself as well. I will make sure to be better with the work I put forth. I am currently working on adding test cases for the work done and would let you know once done with the requested changes. |
Member
|
@AyushiGupta160604 all good, just make sure your AI understands our projects needs so we don't have this back and forth next time :) I do prefer it if contributions are co-authored by AI that you keep it honest like this: 57758f4 <-- note the multiple authors on the commit. |
Member
|
Closing as already dealt with |
JohnMcLear
added a commit
to JohnMcLear/etherpad
that referenced
this pull request
May 13, 2026
…#7666) Takes over ether#7666 / closes ether#7603. Squashed rebase of 32 commits onto current develop (which has since absorbed admin design rework ether#7716 and admin i18n fixes ether#7736 — granular history preserved on takeover/7666-admin-settings-editor before this squash, see PR description for the original commit log). Highlights: - New parsed JSONC settings editor under admin/src/components/settings/ — FormView, ModeToggle, ParseErrorBanner, JsoncNode dispatcher, leaf widgets (string, number, bool, null, env pill), and pure helpers (comments, envPill, jsoncEdit, labels, templateComments). - ${VAR:default} env placeholders render as editable inline inputs that round-trip through the raw textarea (env-pill spec asserts this; docker-template spec protects against form-view degradation on env-heavy configs). - Schema-driven help text sourced from settings.json.template, inlined at build time via vite (drops the runtime fs.allow widening that earlier iterations needed). - ModeToggle switches between FormView and raw textarea on /admin/settings; parse errors surface in a non-blocking banner. - jsonc-parser dep added; pure helpers wrap modify() for stable edits that preserve key order and trailing comments (stops at end-of-line so trailing-comment trains don't bleed into the next property). - i18n keys added for form mode, parse error, env pill, default_label, and input aria. - Playwright specs cover form view, env pill, parse error banner, raw round-trip, and form-mode regressions called out in ether#7666 review (stable React keys from AST offsets, save-toast on server ack only, NumberInput draft sync, parse-error flash during initial load, .settings CSS conflict resolution, focus retention via rAF, IconButton type defaulting to 'button'). Co-authored-by: Ayushi Gupta <ayushigupta36881@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
added a commit
to JohnMcLear/etherpad
that referenced
this pull request
May 15, 2026
…#7666) Takes over ether#7666 / closes ether#7603. Squashed rebase of 32 commits onto current develop (which has since absorbed admin design rework ether#7716 and admin i18n fixes ether#7736 — granular history preserved on takeover/7666-admin-settings-editor before this squash, see PR description for the original commit log). Highlights: - New parsed JSONC settings editor under admin/src/components/settings/ — FormView, ModeToggle, ParseErrorBanner, JsoncNode dispatcher, leaf widgets (string, number, bool, null, env pill), and pure helpers (comments, envPill, jsoncEdit, labels, templateComments). - ${VAR:default} env placeholders render as editable inline inputs that round-trip through the raw textarea (env-pill spec asserts this; docker-template spec protects against form-view degradation on env-heavy configs). - Schema-driven help text sourced from settings.json.template, inlined at build time via vite (drops the runtime fs.allow widening that earlier iterations needed). - ModeToggle switches between FormView and raw textarea on /admin/settings; parse errors surface in a non-blocking banner. - jsonc-parser dep added; pure helpers wrap modify() for stable edits that preserve key order and trailing comments (stops at end-of-line so trailing-comment trains don't bleed into the next property). - i18n keys added for form mode, parse error, env pill, default_label, and input aria. - Playwright specs cover form view, env pill, parse error banner, raw round-trip, and form-mode regressions called out in ether#7666 review (stable React keys from AST offsets, save-toast on server ack only, NumberInput draft sync, parse-error flash during initial load, .settings CSS conflict resolution, focus retention via rAF, IconButton type defaulting to 'button'). Co-authored-by: Ayushi Gupta <ayushigupta36881@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
added a commit
to JohnMcLear/etherpad
that referenced
this pull request
May 15, 2026
…#7666) Takes over ether#7666 / closes ether#7603. Squashed rebase of 32 commits onto current develop (which has since absorbed admin design rework ether#7716 and admin i18n fixes ether#7736 — granular history preserved on takeover/7666-admin-settings-editor before this squash, see PR description for the original commit log). Highlights: - New parsed JSONC settings editor under admin/src/components/settings/ — FormView, ModeToggle, ParseErrorBanner, JsoncNode dispatcher, leaf widgets (string, number, bool, null, env pill), and pure helpers (comments, envPill, jsoncEdit, labels, templateComments). - ${VAR:default} env placeholders render as editable inline inputs that round-trip through the raw textarea (env-pill spec asserts this; docker-template spec protects against form-view degradation on env-heavy configs). - Schema-driven help text sourced from settings.json.template, inlined at build time via vite (drops the runtime fs.allow widening that earlier iterations needed). - ModeToggle switches between FormView and raw textarea on /admin/settings; parse errors surface in a non-blocking banner. - jsonc-parser dep added; pure helpers wrap modify() for stable edits that preserve key order and trailing comments (stops at end-of-line so trailing-comment trains don't bleed into the next property). - i18n keys added for form mode, parse error, env pill, default_label, and input aria. - Playwright specs cover form view, env pill, parse error banner, raw round-trip, and form-mode regressions called out in ether#7666 review (stable React keys from AST offsets, save-toast on server ack only, NumberInput draft sync, parse-error flash during initial load, .settings CSS conflict resolution, focus retention via rAF, IconButton type defaulting to 'button'). Co-authored-by: Ayushi Gupta <ayushigupta36881@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
added a commit
to JohnMcLear/etherpad
that referenced
this pull request
May 15, 2026
…#7666) Takes over ether#7666 / closes ether#7603. Squashed rebase of 32 commits onto current develop (which has since absorbed admin design rework ether#7716 and admin i18n fixes ether#7736 — granular history preserved on takeover/7666-admin-settings-editor before this squash, see PR description for the original commit log). Highlights: - New parsed JSONC settings editor under admin/src/components/settings/ — FormView, ModeToggle, ParseErrorBanner, JsoncNode dispatcher, leaf widgets (string, number, bool, null, env pill), and pure helpers (comments, envPill, jsoncEdit, labels, templateComments). - ${VAR:default} env placeholders render as editable inline inputs that round-trip through the raw textarea (env-pill spec asserts this; docker-template spec protects against form-view degradation on env-heavy configs). - Schema-driven help text sourced from settings.json.template, inlined at build time via vite (drops the runtime fs.allow widening that earlier iterations needed). - ModeToggle switches between FormView and raw textarea on /admin/settings; parse errors surface in a non-blocking banner. - jsonc-parser dep added; pure helpers wrap modify() for stable edits that preserve key order and trailing comments (stops at end-of-line so trailing-comment trains don't bleed into the next property). - i18n keys added for form mode, parse error, env pill, default_label, and input aria. - Playwright specs cover form view, env pill, parse error banner, raw round-trip, and form-mode regressions called out in ether#7666 review (stable React keys from AST offsets, save-toast on server ack only, NumberInput draft sync, parse-error flash during initial load, .settings CSS conflict resolution, focus retention via rAF, IconButton type defaulting to 'button'). Co-authored-by: Ayushi Gupta <ayushigupta36881@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JohnMcLear
added a commit
that referenced
this pull request
May 15, 2026
…7709) * admin: parsed JSONC settings editor with form view (#7603, #7666) Takes over #7666 / closes #7603. Squashed rebase of 32 commits onto current develop (which has since absorbed admin design rework #7716 and admin i18n fixes #7736 — granular history preserved on takeover/7666-admin-settings-editor before this squash, see PR description for the original commit log). Highlights: - New parsed JSONC settings editor under admin/src/components/settings/ — FormView, ModeToggle, ParseErrorBanner, JsoncNode dispatcher, leaf widgets (string, number, bool, null, env pill), and pure helpers (comments, envPill, jsoncEdit, labels, templateComments). - ${VAR:default} env placeholders render as editable inline inputs that round-trip through the raw textarea (env-pill spec asserts this; docker-template spec protects against form-view degradation on env-heavy configs). - Schema-driven help text sourced from settings.json.template, inlined at build time via vite (drops the runtime fs.allow widening that earlier iterations needed). - ModeToggle switches between FormView and raw textarea on /admin/settings; parse errors surface in a non-blocking banner. - jsonc-parser dep added; pure helpers wrap modify() for stable edits that preserve key order and trailing comments (stops at end-of-line so trailing-comment trains don't bleed into the next property). - i18n keys added for form mode, parse error, env pill, default_label, and input aria. - Playwright specs cover form view, env pill, parse error banner, raw round-trip, and form-mode regressions called out in #7666 review (stable React keys from AST offsets, save-toast on server ack only, NumberInput draft sync, parse-error flash during initial load, .settings CSS conflict resolution, focus retention via rAF, IconButton type defaulting to 'button'). Co-authored-by: Ayushi Gupta <ayushigupta36881@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(admin): stabilise React keys to prevent focus loss in settings editor Switch React keys in JsoncNode and FormView from byte offsets to stable JSON paths (`getNodePath(...).join('.')`). Byte offsets shift on every keystroke because the edit changes the surrounding character count, which forces React to remount inputs and lose focus mid-typing. - Object children key on the property path. - Array elements key on their JSON path index. - Add a Playwright regression test pinning focus stability for array element edits. Co-authored-by: John McLear <john@mclear.co.uk> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(admin): stop trailing /* */ comments from bleeding into next key's label (#7740) In the parsed settings form view, each key's row was rendering its label as the previous keys' source lines concatenated together. Root cause: findLeading() in admin/src/components/settings/comments.ts treated any line ending in `*/` as a comment continuation, so a JSON line like "altF9": true, /* focus on the File Menu and/or editbar */ was absorbed into the next sibling's leading comment block, and then each subsequent key picked up an even longer accumulation. - Tighten findLeading's isComment check to only match structural comment lines (`//`, `/*`, or a `*`-prefixed continuation/close), so JSON code with a trailing block comment no longer matches. - Surface leading and trailing comments separately from the template map. Leaf rows with only a trailing same-line comment now render the humanized key as the row label and the comment as the help text below the control, matching settings.json.template's convention (and #7740's recommendation that "helper text should be below"). - Add unit tests pinning the regression and the JSDoc/`//` leading styles, plus a Playwright spec that asserts altC's row carries a clean label and the "focus on the Chat window" help text. Closes #7740. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Ayushi Gupta <ayushigupta36881@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses the "horrific messy blob" issue in the Admin Settings page by transforming the plain textarea into a more functional, developer-friendly editor.
The primary goal was to improve the legibility of the configuration file and preserve the critical documentation (comments) that explain individual settings, which were previously being stripped or were difficult to read.
Purpose of the Change:
Documentation Preservation: By ensuring comments are no longer stripped from the view, admins can now read the built-in documentation for each setting directly in the browser.
Structural Clarity: Utilizing a monospaced font and dark-mode styling ensures that JSON nesting and indentation are perfectly aligned, solving the "messy blob" effect.
Improved Authoring: Adding Tab-key support for indentation and JSON syntax validation with helpful error messages significantly improves the UX when making manual changes to the configuration.
Changes
UI/UX: Replaced the standard unstyled textarea with a dark-themed, monospaced editor view.
Functionality: Added a keyboard handler to allow the Tab key to insert spaces instead of losing focus.
Safety: Implemented a try/catch validation block in the save function to alert users of JSON syntax errors (like missing commas or braces) before they attempt to save and restart the server.
New Feature: Added a "Prettify JSON" button to auto-format standard JSON strings for better readability.
Future Considerations
fixes #7603